Skip to content
This repository has been archived by the owner on Feb 17, 2022. It is now read-only.

[WIP] SDL2 thread proxying fixes #127

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jakogut
Copy link
Contributor

@jakogut jakogut commented Jul 23, 2020

This PR uses new APIs added in emscripten-core/emscripten#9336 to improve compatibility with USE_PTHREADS=1.

@jakogut
Copy link
Contributor Author

jakogut commented Jul 23, 2020

This isn't actually WIP, but it does require emscripten-core/emscripten#9336 to land before merging.

Copy link
Member

@Daft-Freak Daft-Freak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two that use the new APIs are definitely a nice cleanup. Might want to change the identical commit messages on the two mouse ones ("cursor creation"/"cursor setting"?), that confused me for a bit.

Not sure if there's a reason this doesn't just use MAIN_THREAD_EM_ASM* everywhere? (Haven't done anything with threads yet.)

@@ -209,7 +233,7 @@ Emscripten_ShowCursor(SDL_Cursor* cursor)
curdata = (Emscripten_CursorData *) cursor->driverdata;

if(curdata->system_cursor) {
EM_ASM_INT({
MAIN_THREAD_EM_ASM_INT({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch this to MAIN_THREAD_EM_ASM and drop the return. (I think EM_ASM was less flexible when this was written.)

hot_y,
conv_surf->pixels
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly annoying that we have to proxy this as it just wants to create an image from some data... But I don't think it can be avoided.

@uyjulian
Copy link

You might want to add SDL_GetDisplayUsableBounds to the functions to be proxied also…

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants